Add QueueManager & improve TaskQueue#393
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a QueueManager class to manage multiple throttling queues and refactors TaskQueue to remove the p-limit dependency by implementing custom concurrency control. The changes also optimize canvas operations by adding willReadFrequently: true to 2D context creation.
- Introduces
QueueManagerfor convenient management of multiple per-host throttling queues - Refactors
TaskQueueconcurrency logic to provide proper back pressure and remove external dependency - Adds canvas optimization flags to fix Chrome warnings and potential buffer corruption
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/task-herder/src/QueueManager.ts | New class for managing multiple named task queues with shared/individual configurations |
| packages/task-herder/test/manager.test.ts | Comprehensive test coverage for QueueManager API including create, get, and getOrCreate methods |
| packages/task-herder/src/index.ts | Exports the new QueueManager class |
| packages/task-herder/src/TaskQueue.ts | Refactored concurrency control with custom implementation, removed p-limit dependency, added options getter |
| packages/task-herder/package.json | Removed p-limit dependency |
| package-lock.json | Removed p-limit and yocto-queue dependencies from task-herder |
| packages/scratch-vm/src/import/load-costume.js | Added willReadFrequently flag to canvas 2D context for bitmap loading |
| packages/scratch-render/src/BitmapSkin.js | Added willReadFrequently flag with explanatory comment for WebGL texture data extraction |
| packages/scratch-gui/webpack.config.js | Added vendor chunks copying for scratch-storage npm link support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Test report for scratch-svg-renderer 1 files 60 suites 0s ⏱️ Results for commit 62a4800. ♻️ This comment has been updated with latest results. |
Test report for task-herder28 tests 28 ✅ 0s ⏱️ Results for commit 62a4800. ♻️ This comment has been updated with latest results. |
Test report for scratch-render 1 files 55 suites 2s ⏱️ Results for commit 62a4800. ♻️ This comment has been updated with latest results. |
Test report for scratch-vm 1 files 763 suites 1m 6s ⏱️ Results for commit 62a4800. ♻️ This comment has been updated with latest results. |
This fixes a Chrome crash when loading assets with moderately high concurrency. It's also allegedly a good idea for performance.
981844b to
1be24de
Compare
KManolov3
left a comment
There was a problem hiding this comment.
This looks good to me! Thank you for adding tests!
Resolves
Supports an upcoming
scratch-storagechange that will resolve scratchfoundation/scratch-gui#7111 and POD-238Proposed Changes
Main change: in
task-herder, addQueueManager, a way to conveniently manage multiple throttling queues.Supporting changes:
QueueManagerTaskQueuescratch-storage, which helps when usingnpm link scratch-storage{willReadFrequently: true}to theCanvas.getContext('2d')call in the bitmap load pathReason for Changes
scratch-storagechange will use this newQueueManagerto manage per-host throttling queues.p-limitdependency is a nice side effect. (There's nothing wrong withp-limit, but fewer dependencies is better.)willReadFrequentlychange:Test Coverage
The new
QueueManageris covered by new tests. Existing tests cover everything else.